-
Notifications
You must be signed in to change notification settings - Fork 283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(copm): add fabric COPM implementation #3546
base: main
Are you sure you want to change the base?
Conversation
2342278
to
a8ae134
Compare
|
||
message AssetAccountV1PB { | ||
|
||
string network = 232872497; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't have context about this structure, but why are these numbers so high?
Usually proto structures start with 1
, 2
and so on...
Same comment for all the below proto files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pb files are generated from openapi, following cactus convention. The openapi desc is here: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/,
|
||
message ViewAddressV1PB { | ||
|
||
string contract_id = 512823451; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need more fields to this, but I'm assuming, we will keep updating this in future with implementations of these services?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For now I want to see what we can get away with. In Corda, flowpath (for instance) is very similar to a contract specification.
|
||
import "models/view_address_v1_pb.proto"; | ||
|
||
message ViewV1PB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View contains response to the remote query right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is part of the inbound message for the verifyState command. You can see the messages here for more context: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenniferlianne Very high quality PR overall, thank you very much for the contribution! I left a few comments with my usual nit picks but it's definitely looking great already! Congratulations!
...ages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts
Show resolved
Hide resolved
c50865d
to
48b4e82
Compare
"post": { | ||
"x-hyperledger-cacti": { | ||
"http": { | ||
"path": "/api/v1/plugins/@hyperledger/cacti-plugin-copm/prove-state", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path is different from "/api/v1/plugins/@hyperledger/cacti-plugin-copm/verify-state"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Endpoint has been updated to reflect naming recently discussed.
"verbLowerCase": "post" | ||
} | ||
}, | ||
"operationId": "provestateV1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use similar name as API names and IDs, so to be consistent with other API names and IDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
|
||
rpc PledgeAssetV1 (PledgeAssetV1Request) returns (PledgeAssetV1200ResponsePB); | ||
|
||
rpc ProvestateV1 (ProvestateV1Request) returns (google.protobuf.Empty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Name isn't updated here
|
||
} | ||
|
||
message ProvestateV1Request { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Name isn't updated here
} from "@hyperledger/cacti-copm-core"; | ||
import { Logger } from "@hyperledger/cactus-common"; | ||
|
||
export async function proveStateV1Impl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also function name I think should match with API name?
} | ||
} | ||
|
||
public async provestateV1(req: ProvestateV1Request): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here too
48b4e82
to
ab02c7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenniferlianne LGTM, thank you very much!
@jenniferlianne Can you also check the documentation (with names updated as per your discussion with @sandeepnRES) from https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/ into this PR? That way, when we add code to the main repo, there is (at least some) documentation explaining what it does. (BTW....sorry for my late comments. I've still not recovered from my illness, and am taking it slow.) |
The openapi documentation should be generated under this link https://hyperledger-cacti.github.io/cacti/references/openapi/ when the code is merged. An updated preview (with the changes discussed) is here: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/ |
Primary Change: - add connectRPC cactus plugin for copm primitives Secondary Changes: - add common COPM prototypes - add common weaver protos-js directory to enable building under ci.yaml, which requires all local dependencies to be present at build time - remove incorrect 'main' declaration in protos-js package file - add separate tsconfig file to /weaver/sdks/fabric/interoperation-node-sdk to allow it to be built by ci.yaml - update weaver fabric network makefile 'clean' target to continue if some target files were not generated by the make process Signed-off-by: Jennifer Bell <jenniferlianne@gmail.com>
ab02c7f
to
c3daece
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Primary Change:
Secondary Changes:
dependencies to be present at build time
allow it to be built by ci.yaml
were not generated by the make process
Signed-off-by: Jennifer Bell jenniferlianne@gmail.com
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.